Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

new unstable option: -Zwrite-long-types-to-disk #113893

Merged
merged 2 commits into from
Jul 26, 2023
Merged

new unstable option: -Zwrite-long-types-to-disk #113893

merged 2 commits into from
Jul 26, 2023

Conversation

mdibaiee
Copy link
Contributor

This option guards the logic of writing long type names in files and instead using short forms in error messages in rustc_middle/ty/error behind a flag. The main motivation for this change is to disable this behaviour when running ui tests.

This logic can be triggered by running tests in a directory that has a long enough path, e.g. /my/very-long-path/where/rust-codebase/exists/

This means ui tests can fail depending on how long the path to their file is.

Some ui tests actually rely on this behaviour for their assertions, so for those we enable the flag manually.

@rustbot
Copy link
Collaborator

rustbot commented Jul 20, 2023

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 20, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@pnkfelix
Copy link
Member

It looks like you missed updating the stderr for tests/ui/recursion/issue-83150.rs

I plan to help this PR move along by contributing patches to it (I tend to have long paths to my working directories, so landing this will help unblock my own development)

@mdibaiee mdibaiee force-pushed the type-name-spill-flag branch 3 times, most recently from 01103b5 to 8ffbd80 Compare July 23, 2023 08:44
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@mdibaiee mdibaiee force-pushed the type-name-spill-flag branch from 8ffbd80 to 0edb3e9 Compare July 24, 2023 08:28
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@mdibaiee
new unstable option: -Zwrite-long-types-to-disk
8df3966 
This option guards the logic of writing long type names in files and
instead using short forms in error messages in rustc_middle/ty/error
behind a flag. The main motivation for this change is to disable this
behaviour when running ui tests.

This logic can be triggered by running tests in a directory that has a
long enough path, e.g. /my/very-long-path/where/rust-codebase/exists/

This means ui tests can fail depending on how long the path to their
file is.

Some ui tests actually rely on this behaviour for their assertions,
so for those we enable the flag manually.
@mdibaiee mdibaiee force-pushed the type-name-spill-flag branch from 0edb3e9 to 8df3966 Compare July 24, 2023 11:25
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@mdibaiee
Copy link
Contributor Author

r? @compiler-errors

@rustbot rustbot assigned compiler-errors and unassigned TaKO8Ki Jul 24, 2023
@mdibaiee
Copy link
Contributor Author

I can't figure out what the root cause of this last error I am getting is (to be fair I didn't put too much time investigating). The option I have added is untracked, but the error seems to be related to tracked options. 🤨 If someone can explain I can see about resolving it

@mdibaiee mdibaiee mentioned this pull request Jul 24, 2023
Closed
compiler-errors
compiler-errors requested changes Jul 24, 2023
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good from the compiler side, just had one concern about UI tests:

Most of these tests don't seem to be testing long-type-names. Can you remove -Zwrite-long-types-to-disk=yes from all of them except for like long-E0308.rs, and all the other normalization stuff like

// normalize-stderr-test: "long-type-\d+" -> "long-type-hash"

and bless all the tests?

@mdibaiee
Copy link
Contributor Author

@compiler-errors done! I initially added write-long-types-to-disk=yes to some other tests because I figured they always will produce long-enough types that would trigger the logic. I have now reverted that change and now only the test for this specific logic uses this flag.

@mdibaiee mdibaiee marked this pull request as ready for review July 25, 2023 11:46
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 25, 2023

📌 Commit b2d052b has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 25, 2023
@bors
Copy link
Contributor

bors commented Jul 26, 2023

⌛ Testing commit b2d052b with merge bd1ae28...

@bors
Copy link
Contributor

bors commented Jul 26, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing bd1ae28 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 26, 2023
@bors bors merged commit bd1ae28 into rust-lang:master Jul 26, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 26, 2023
@mdibaiee mdibaiee deleted the type-name-spill-flag branch July 26, 2023 08:21
@mdibaiee mdibaiee mentioned this pull request Jul 26, 2023
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (bd1ae28): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.0% [-2.0%, -2.0%] 1
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -1.9% [-1.9%, -1.9%] 1
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 650.399s -> 650.255s (-0.02%)

mdibaiee added a commit to mdibaiee/rust that referenced this pull request Jul 26, 2023
@mdibaiee
compiletest: remove ci-specific remap-path-prefix
7d773c3 
Now that we have fixed the underlying cause of long type name
inconsistencies in rust-lang#113893, we can remove the remap-path-prefix logic
from CI
@compiler-errors compiler-errors mentioned this pull request Jul 26, 2023
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 26, 2023
@matthiaskrgr
Rollup merge of rust-lang#114090 - mdibaiee:remove-remap-path-prefix-…
a1956e2 
…ci, r=wesleywiser

compiletest: remove ci-specific remap-path-prefix

Now that we have fixed the underlying cause of long type name inconsistencies in rust-lang#113893, we can remove the remap-path-prefix logic from CI

resolves rust-lang#113424
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 27, 2023
@matthiaskrgr
Rollup merge of rust-lang#114102 - compiler-errors:fulldeps-stage1-ha…
8cae343 
…ck, r=pnkfelix

Dont pass `-Zwrite-long-types-to-disk=no` for `ui-fulldeps --stage=1`

Due to this hack:

https://github.com/rust-lang/rust/blob/601a34de8c10458b72a7781eb0b44a7981e4a2b1/src/bootstrap/test.rs#L1473-L1484

We use the stage 0 compiler to build the stage 1 fulldeps tests. That means that we don't have `-Zwrite-long-types-to-disk=no` which was added in rust-lang#113893.

Add a temporary hack to fix this (https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Some.20tests.20failing.20with.20--stage.201) until the next beta bump.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pnkfelix pnkfelix pnkfelix left review comments

@compiler-errors compiler-errors compiler-errors approved these changes

Assignees

@compiler-errors compiler-errors

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.73.0
Development

Successfully merging this pull request may close these issues.
8 participants
@mdibaiee @rustbot @rust-log-analyzer @pnkfelix @compiler-errors @bors @rust-timer @TaKO8Ki